feat: Support objectOverrides
#1118
Conversation
Techassi
left a comment
Techassi
left a comment
There was a problem hiding this comment.
Mostly lgtm, a few various question, comments and suggestions.
| ### Added | ||
|
|
||
| - Support `objectOverrides` ([#1118]). |
There was a problem hiding this comment.
note: I think we should add a small explanation how this works/what this does.
| ### Removed | ||
|
|
||
| - BREAKING: `ClusterResources` no longer derives `Eq` and `PartialEq` ([#1118]). |
There was a problem hiding this comment.
question: Whats the reason those two trait implementations are removed?
There was a problem hiding this comment.
It's because Vec is now stored in the ClusterResources, and DynamicObject does not implement Eq. I was at least able to derive PartialEq again: bf1d753
| merge_strategies::map::granular( | ||
| &mut self.extra_pod_selector_labels, | ||
| other.extra_pod_selector_labels, | ||
| |current_item, other_item| { | ||
| DeepMerge::merge_from(current_item, other_item); | ||
| }, | ||
| ); | ||
| merge_strategies::list::map( | ||
| &mut self.ports, | ||
| other.ports, | ||
| &[|lhs, rhs| lhs.name == rhs.name], | ||
| |current_item, other_item| { | ||
| DeepMerge::merge_from(current_item, other_item); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
note: Please add two dev comments to quickly explain what this code does and why we need this specialized merge.
There was a problem hiding this comment.
I struggle to find anything that is not already on the trait docs in https://docs.rs/k8s-openapi/latest/k8s_openapi/trait.DeepMerge.html
What are you thinking of?
| merge_strategies::list::map( | ||
| &mut self.ingress_addresses, | ||
| other.ingress_addresses, | ||
| &[|lhs, rhs| lhs.address == rhs.address], | ||
| |current_item, other_item| { | ||
| DeepMerge::merge_from(current_item, other_item); | ||
| }, | ||
| ); | ||
| merge_strategies::map::granular( | ||
| &mut self.node_ports, | ||
| other.node_ports, | ||
| |current_item, other_item| { | ||
| DeepMerge::merge_from(current_item, other_item); | ||
| }, | ||
| ); |
| let Some(patch_type) = &patch.types else { | ||
| return Ok(()); | ||
| }; | ||
| if patch_type.api_version != R::api_version(&()) || patch_type.kind != R::kind(&()) { |
There was a problem hiding this comment.
| if patch_type.api_version != R::api_version(&()) || patch_type.kind != R::kind(&()) { | |
| if object_type.api_version != R::api_version(&()) || object_type.kind != R::kind(&()) { |
| if patch_type.api_version != R::api_version(&()) || patch_type.kind != R::kind(&()) { | ||
| return Ok(()); | ||
| } | ||
| let Some(patch_name) = &patch.metadata.name else { |
There was a problem hiding this comment.
| let Some(patch_name) = &patch.metadata.name else { | |
| let Some(object_name) = &override.metadata.name else { |
|
|
||
| let deserialized_patch = | ||
| patch | ||
| .clone() |
There was a problem hiding this comment.
note: If we need to clone every single override anyway, we should instead take an owned value (in both functions). Taking a reference is signaling that no owned data is needed, which is not true. We should make this clear when these functions are called.
|
|
||
| /// Using [`serde_yaml`] to generate the test data | ||
| fn generate_service_account() -> ServiceAccount { | ||
| serde_yaml::from_str( |
There was a problem hiding this comment.
note: Again, I would like to see this (and all other inline YAML strings) getting moved into actual YAML files.
There was a problem hiding this comment.
I personally prefer having them in-line as this way it's IMHO much easier to read and maintain compared to cluttered over 20 files.
I was thinking of using https://docs.rs/indoc/2.0.7/indoc/, but figured it's not worth the effort, but happy to do so
| /// Generate the test data programmatically (as operators would normally do) | ||
| fn generate_stateful_set() -> StatefulSet { |
There was a problem hiding this comment.
question: But, do we need to? I feel like this can also be a static YAML file (to actually only test the merging/overrides).
There was a problem hiding this comment.
Nope, we don't. I only did this to increase the test diversity 😅
My though process was maybe somethings with defaults with and without serde might be different and stuff.
It's not super important, but I also don't think this will hurt us in any way, so I would say better safe than sorry.
Techassi
moved this from Development: Waiting for Review
to Development: In Review
in Stackable Engineering
Co-authored-by: Techassi <[email protected]>
Description
Part of stackabletech/issues#712
Decision: https://github.com/stackabletech/decisions/issues/64
Needs stackabletech/documentation#807 for the documentation part
The actual code change is oversee-able, most of the added lines are tests.
Definition of Done Checklist
Author
Reviewer
Acceptance